Skip to content

Support for renaming JS methods when re-exporting#1010

Open
palas wants to merge 2 commits intomasterfrom
support-for-renaming-js-functions
Open

Support for renaming JS methods when re-exporting#1010
palas wants to merge 2 commits intomasterfrom
support-for-renaming-js-functions

Conversation

@palas
Copy link
Contributor

@palas palas commented Nov 13, 2025

Changelog

- description: |
    Add support for renaming JS methods re-exporting and use the functionality to simplify wallet testnet methods
  type:
  - breaking
  projects:
  - cardano-wasm

Context

See #1003 (comment). Names of functions exported by the API need to be unique, but we can rename them as we re-export them for the JS API function.

How to trust this PR

The tests passing gives strong guarantee the renaming is done correctly. Other than that, it could be that there is some file that wasn't updated that we missed. And also it would be a matter of checking whether the comments and code design is sensible.

I would look at the commit separately too, because the first one is a refactoring, and the second one is a simple change.

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated. See Running tests for more details
  • Self-reviewed the diff

@palas palas self-assigned this Nov 13, 2025
Jimbo4350
Jimbo4350 requested changes Nov 14, 2025
View reviewed changes
Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the methodParams field is it possible to encode the idea of sum type? I.e require a network magic number or default to mainnet?

@palas
Copy link
Contributor Author

palas commented Nov 14, 2025

In the methodParams field is it possible to encode the idea of sum type? I.e require a network magic number or default to mainnet?

@Jimbo4350, the easiest we could do is have a nullable magic number parameter (if it is the last parameter I think it could also be optional), and if you don't provide it we can default to mainnet. Still we can want the renaming functionality. Because that way we can have methods with the same name in different objects, like toCbor, otherwise we'll need txToCbor and certToCbor, because they are in the same namespace. This will still be the case at the WASM level, but we can structure them better for JS

@palas palas requested a review from Jimbo4350 November 14, 2025 17:31
carbolymer
carbolymer reviewed Nov 28, 2025
View reviewed changes
cardano-wasm/src-lib/Cardano/Wasm/Api/Info.hs
{ methodName :: String
-- ^ Name of the method in the virtual object of the JS API (which should match the exported function).
-- ^ Name of the global method name in the API (which should match the exported function and it must be unique globally).
, methodSimpleName :: String
Copy link
Contributor

@carbolymer carbolymer Nov 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
, methodSimpleName :: String
, methodSimpleName :: Maybe String

I think it should default to methodName. I'd throw

instance IsString s => IsString (Maybe s) where 
  fromString = Just . fromString

into the mix with OverloadedStrings. I think we tried that in the past, but it was too magical to the tweag folks.

carbolymer
carbolymer reviewed Nov 28, 2025
View reviewed changes
cardano-wasm/src-lib/Cardano/Wasm/Api/Info.hs
, MethodInfoEntry $
MethodInfo
{ methodName = "estimateMinFee"
, methodSimpleName = "estimateMinFee"
Copy link
Contributor

@carbolymer carbolymer Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those duplicated fields look like a good use case for some kind of smart constructor. But then you have unnamed fields and it would be nice to undo that with something like named but then I guess it turns into a new can of worms. 😄

@github-actions
Copy link

github-actions bot commented Jan 13, 2026

This PR is stale because it has been open 45 days with no activity.

@github-actions github-actions bot added the Stale label Jan 13, 2026
@palas palas removed the Stale label Jan 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@carbolymer carbolymer carbolymer left review comments

@dcoutts dcoutts Awaiting requested review from dcoutts dcoutts is a code owner

@erikd erikd Awaiting requested review from erikd erikd is a code owner

@newhoggy newhoggy Awaiting requested review from newhoggy newhoggy is a code owner

@kevinhammond kevinhammond Awaiting requested review from kevinhammond kevinhammond is a code owner

@CarlosLopezDeLara CarlosLopezDeLara Awaiting requested review from CarlosLopezDeLara CarlosLopezDeLara is a code owner

@disassembler disassembler Awaiting requested review from disassembler disassembler is a code owner

@Jimbo4350 Jimbo4350 Awaiting requested review from Jimbo4350 Jimbo4350 is a code owner

Requested changes must be addressed to merge this pull request.

Assignees

@palas palas

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@palas @carbolymer @Jimbo4350